stats: strip inner custom namespaces in prometheus names#45065
stats: strip inner custom namespaces in prometheus names#45065yueshangzuo wants to merge 1 commit into
Conversation
yueshangzuo
force-pushed
the
fix/scoped-custom-namespace-prometheus
branch
2 times, most recently
from
8c3fbaf to
e2cb747
Compare
|
/retest |
|
I think the issue might be that the Wasm was using a nested scope? Wasm lifecycle is decoupled from the xDS lifecycle so I think it might be better to place it under the root stats scope for all extensions? |
|
/assign-from @envoyproxy/envoy-maintainers |
|
@envoyproxy/envoy-maintainers assignee is @nezdolik |
|
@kyessenov This PR keeps the existing scoping behavior and only fixes the Prometheus formatting side, gated by |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Prometheus stats formatter to strip registered custom stat namespaces from inner segments of stat names, ensuring that metrics like upstream Wasm counters render with the standard envoy_ prefix and without internal namespaces. This change is controlled by the envoy.reloadable_features.strip_scoped_custom_stat_namespace runtime guard. A critical issue was identified in the implementation of stripRegisteredInnerNamespace where an undefined method registered was used instead of checking the namespaces_ set directly.
yueshangzuo
force-pushed
the
fix/scoped-custom-namespace-prometheus
branch
from
e2cb747 to
df74d9f
Compare
|
/retest |
nezdolik
left a comment
nezdolik
left a comment
There was a problem hiding this comment.
lgtm, @kyessenov do you have any more feedback on the approach?
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
drive-by readability comment.
The Prometheus formatter only stripped a registered custom stat namespace when it was the leading segment of the tag-extracted name. This worked for listener/root-scoped Wasm custom stats but not for upstream Wasm stats scoped under a cluster, where after tag extraction the namespace ends up in the middle (e.g. `cluster.wasmcustom.foo`), producing names like `envoy_cluster_wasmcustom_foo`. Add `CustomStatNamespaces::stripRegisteredInnerNamespace()` and call it from the Prometheus formatter to strip a registered namespace appearing as a non-leading, non-trailing segment, without hard-coding `wasmcustom` or a specific scope depth. Gated by `envoy.reloadable_features.strip_scoped_custom_stat_namespace` (default true). Signed-off-by: Yueshang zuo <zuoyueshang.zys@alibaba-inc.com>
yueshangzuo
force-pushed
the
fix/scoped-custom-namespace-prometheus
branch
from
df74d9f to
582c822
Compare
|
/retest |
|
@nezdolik Friendly ping when you get a chance. |
|
Agree with @kyessenov, the problem is that the upstream wasm filter should use root based stats scope like downstream wasm filter rather than to use cluster prefixed scope. |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
Is that possible to fix WASM's scope directly?
|
Superseded by #45242, which addresses this at the upstream Wasm stats scope selection layer instead of in the Prometheus formatter. |
Supersedes #45065. Implements the root-scope approach surfaced in that review discussion. Commit Message: wasm: use root stats scope for upstream filter stats Additional Description: Upstream HTTP Wasm filter stats currently use the cluster stats scope, while downstream HTTP Wasm filter stats use the root scope. This makes the same Wasm metric name produce different stat names depending on whether the plugin is loaded as a downstream or upstream filter. This changes upstream HTTP Wasm filter stats to use the server-wide root stats scope by default, matching the downstream filter. In Prometheus, a counter previously exposed as `envoy_cluster_wasmcustom_foo{envoy_cluster_name="X"}` is now exposed as `foo`. The previous cluster-scope behavior can be temporarily restored with the `envoy.reloadable_features.upstream_wasm_filter_uses_root_scope` runtime guard set to `false`. Risk Level: Medium Testing: - `git diff --check` - `bazel test -c dbg //test/extensions/filters/http/wasm:config_test` - `bazel build -c dbg envoy` - Manual Envoy smoke test with upstream Wasm stats and runtime-guard rollback. Docs Changes: N/A Release Notes: Added in `changelogs/current/minor_behavior_changes/wasm__upstream-http-wasm-stats-use-root-scope.rst`. Platform Specific Features: N/A [Optional Runtime guard:] `envoy.reloadable_features.upstream_wasm_filter_uses_root_scope`, defaults to true. Set to false to restore the previous upstream Wasm cluster-scope stats behavior. [Optional Fixes #Issue:] N/A [Optional Deprecated:] N/A [Optional API Considerations:] N/A [Optional AI Disclosure:] This PR was prepared with assistance from generative AI; the code and behavior were reviewed and tested by the author. Signed-off-by: Yueshang zuo <zuoyueshang.zys@alibaba-inc.com>
Supersedes envoyproxy#45065. Implements the root-scope approach surfaced in that review discussion. Commit Message: wasm: use root stats scope for upstream filter stats Additional Description: Upstream HTTP Wasm filter stats currently use the cluster stats scope, while downstream HTTP Wasm filter stats use the root scope. This makes the same Wasm metric name produce different stat names depending on whether the plugin is loaded as a downstream or upstream filter. This changes upstream HTTP Wasm filter stats to use the server-wide root stats scope by default, matching the downstream filter. In Prometheus, a counter previously exposed as `envoy_cluster_wasmcustom_foo{envoy_cluster_name="X"}` is now exposed as `foo`. The previous cluster-scope behavior can be temporarily restored with the `envoy.reloadable_features.upstream_wasm_filter_uses_root_scope` runtime guard set to `false`. Risk Level: Medium Testing: - `git diff --check` - `bazel test -c dbg //test/extensions/filters/http/wasm:config_test` - `bazel build -c dbg envoy` - Manual Envoy smoke test with upstream Wasm stats and runtime-guard rollback. Docs Changes: N/A Release Notes: Added in `changelogs/current/minor_behavior_changes/wasm__upstream-http-wasm-stats-use-root-scope.rst`. Platform Specific Features: N/A [Optional Runtime guard:] `envoy.reloadable_features.upstream_wasm_filter_uses_root_scope`, defaults to true. Set to false to restore the previous upstream Wasm cluster-scope stats behavior. [Optional Fixes #Issue:] N/A [Optional Deprecated:] N/A [Optional API Considerations:] N/A [Optional AI Disclosure:] This PR was prepared with assistance from generative AI; the code and behavior were reviewed and tested by the author. Signed-off-by: Yueshang zuo <zuoyueshang.zys@alibaba-inc.com>
5 participants
Commit Message:
stats: strip inner custom namespaces in prometheus names
The Prometheus formatter only stripped a registered custom stat namespace when it was the leading segment of the tag-extracted name. This worked for listener/root-scoped Wasm custom stats but not for upstream Wasm stats scoped under a cluster, where after tag extraction the namespace ends up in the middle (e.g.
cluster.wasmcustom.foo), producing names likeenvoy_cluster_wasmcustom_foo.Add
CustomStatNamespaces::stripRegisteredInnerNamespace()and call it from the Prometheus formatter to strip a registered namespace appearing as a non-leading, non-trailing segment, without hard-codingwasmcustomor a specific scope depth. Gated byenvoy.reloadable_features.strip_scoped_custom_stat_namespace(default true).Additional Description:
The existing leading-prefix custom namespace behavior is unchanged. This aligns upstream Wasm custom stats with listener/root-scoped Wasm custom stats and native cluster metrics without hard-coding
wasmcustomor a specific scope depth in the Prometheus formatter.This PR was prepared with assistance from generative AI; the code and behavior were reviewed and tested by the author.
Risk Level:
Medium
Testing:
Added unit coverage for custom namespace inner-segment stripping and Prometheus formatting, including nested scope names, runtime guard disabled behavior, leading-prefix behavior, unregistered namespaces, and full Prometheus text output.
Docs Changes:
N/A
Release Notes:
See changelogs/current.yaml.
Platform Specific Features:
N/A
[Optional Runtime guard:]
envoy.reloadable_features.strip_scoped_custom_stat_namespace
[Optional API Considerations:]
Adds a new pure virtual method
stripRegisteredInnerNamespace()toStats::CustomStatNamespaces. The interface is internal to the stats subsystem and only one production implementation exists (CustomStatNamespacesImpl); no mocks override it, so impact is contained to this PR.